[BB-1409] Add caching to reduce API calls #107
Conversation
WalkthroughAdds internal user and team caches, wires them through builders and resource types, replaces direct GitHub lookups with cache-backed access, and standardizes rate-limit error wrapping to include response metadata. Initializes caches in connector setup and updates related syncers and builders accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Resource (Repo/Org/Team/User)
participant UC as UserCache
participant TC as TeamCache
participant GH as GitHub API
participant EH as Error Wrapper
R->>UC: GetUser(id/login)
alt Cache hit
UC-->>R: User
else Cache miss
UC->>GH: Fetch User
alt Success
GH-->>UC: User + Resp
UC-->>R: User (cached)
else Rate-limited / error
GH-->>UC: Error + Resp
UC->>EH: WrapErrorsWithRateLimitInfo(resp, err)
EH-->>R: Unavailable error with rate-limit info
end
end
Note over R,TC: GetTeam(id) follows analogous flow via TeamCache
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
aldevv
force-pushed
the
improve_api_calls
branch
from
c87b79a to
0be9a64
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connector/helpers.go (1)
141-177: Address the TODO for deprecated API usage.The
teamDataCacheimplementation is correct, but the TODO comment at line 162 indicates thatGetTeamByIDis deprecated and should be migrated toGetTeamBySlug.Do you want me to track this migration task by opening a new issue, or would you prefer to address it in this PR?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
pkg/connector/api_token.go(1 hunks)pkg/connector/connector.go(4 hunks)pkg/connector/enterprise_role.go(3 hunks)pkg/connector/helpers.go(1 hunks)pkg/connector/org.go(8 hunks)pkg/connector/org_role.go(6 hunks)pkg/connector/org_role_test.go(2 hunks)pkg/connector/org_test.go(1 hunks)pkg/connector/repository.go(8 hunks)pkg/connector/repository_test.go(1 hunks)pkg/connector/team.go(8 hunks)pkg/connector/team_test.go(1 hunks)pkg/connector/user.go(5 hunks)pkg/connector/user_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/user.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (39)
pkg/connector/api_token.go (1)
98-98: LGTM!The update to
WrapErrorsWithRateLimitInfoadds rate-limit metadata to the unavailable error response, aligning with the broader PR pattern for standardizing rate-limit error handling.pkg/connector/helpers.go (1)
70-139: LGTM!The
userDataCacheimplementation is correct:
- Double-checked locking is properly implemented in both
GetUserandGetUserByLogin.- Cache coherence is maintained by populating both
usersByIdandusersByLoginmaps on cache miss.- Error handling and early return on cache hit ensure minimal lock contention.
pkg/connector/user_test.go (1)
47-54: LGTM!The test correctly constructs and injects the
userCacheintouserBuilder, aligning with the broader PR pattern of cache-backed resource access.pkg/connector/org_test.go (1)
26-27: LGTM!The test correctly constructs and injects the
userCacheintoorgBuilder, aligning with the broader PR pattern of cache-backed resource access.pkg/connector/org_role_test.go (2)
27-28: LGTM!The test correctly constructs and injects the
userCacheintoorgRoleBuilder, aligning with the broader PR pattern of cache-backed resource access.
93-94: LGTM!The second test case correctly constructs and injects the
userCacheintoorgRoleBuilder, consistent with the first test case and the broader PR pattern.pkg/connector/team_test.go (1)
27-29: LGTM!The test correctly constructs and injects both
userCacheandteamCacheintoteamBuilder, aligning with the broader PR pattern of cache-backed resource access.pkg/connector/org.go (7)
41-41: LGTM!The addition of the
userCachefield toorgResourceTypecorrectly wires the user data cache through the type, enabling cache-backed user lookups.
107-107: LGTM!The update to
WrapErrorsWithRateLimitInfoadds rate-limit metadata to the unavailable error response, aligning with the broader PR pattern.
135-135: LGTM!The update to
WrapErrorsWithRateLimitInfoadds rate-limit metadata to the unavailable error response, consistent with the broader PR pattern.
233-233: LGTM!The update to
WrapErrorsWithRateLimitInfoadds rate-limit metadata to the unavailable error response, consistent with the broader PR pattern.
298-298: LGTM!Replacing the direct
GetByIDcall withuserCache.GetUsercorrectly leverages the cache to reduce API calls, aligning with the PR objective.
390-390: LGTM!Replacing the direct
GetByIDcall withuserCache.GetUsercorrectly leverages the cache to reduce API calls, consistent with the change at line 298.
420-436: LGTM!The
orgBuildercorrectly accepts and stores theuserCache, ensuringorgResourceTypehas access to cached user data for Grant and Revoke operations.pkg/connector/team.go (9)
63-64: LGTM!The addition of
userCacheandteamCachefields toteamResourceTypecorrectly wires the caches through the type, enabling cache-backed lookups.
101-101: LGTM!The update to
WrapErrorsWithRateLimitInfoadds rate-limit metadata to the unavailable error response, aligning with the broader PR pattern.
112-118: LGTM!Replacing the direct
GetTeamByIDcall withteamCache.GetTeamcorrectly leverages the cache to reduce API calls, aligning with the PR objective.
115-115: LGTM!The update to
WrapErrorsWithRateLimitInfoadds rate-limit metadata to the unavailable error response, consistent with the broader PR pattern.
178-178: LGTM!The update to
WrapErrorsWithRateLimitInfoadds rate-limit metadata to the unavailable error response, consistent with the broader PR pattern.
217-217: LGTM!The update to
WrapErrorsWithRateLimitInfoadds rate-limit metadata to the unavailable error response, consistent with the broader PR pattern.
307-307: LGTM!Replacing the direct
GetByIDcall withuserCache.GetUsercorrectly leverages the cache to reduce API calls, aligning with the PR objective.
367-367: LGTM!Replacing the direct
GetByIDcall withuserCache.GetUsercorrectly leverages the cache to reduce API calls, consistent with the change at line 307.
379-387: LGTM!The
teamBuildercorrectly accepts and stores bothuserCacheandteamCache, ensuringteamResourceTypehas access to cached data for all team-related operations.pkg/connector/repository_test.go (1)
27-29: LGTM!The test correctly instantiates the new
userCacheandteamCacheand passes them torepositoryBuilder, aligning with the updated constructor signature.pkg/connector/connector.go (4)
100-101: LGTM!The new
userCacheandteamCachefields in theGitHubstruct provide a clean way to store and pass cache instances through the connector's lifecycle.
108-126: LGTM!All resource syncers are correctly wired with the new
userCacheandteamCacheinstances, enabling cache-backed lookups across the board.
320-321: LGTM!The caches are initialized cleanly alongside the existing
orgCache, ensuring they're ready for use when builders are constructed.
462-462: LGTM!Updating the error wrapping to
WrapErrorsWithRateLimitInfostandardizes rate-limit error reporting and includes useful response metadata for observability.pkg/connector/org_role.go (4)
43-43: LGTM!Adding
userCachetoorgRoleResourceTypefollows the consistent pattern across all resource types in this PR.
95-95: LGTM!The updated error wrapping consistently includes rate-limit information from the response, improving observability and debugging.
Also applies to: 232-232
329-329: LGTM!Replacing direct GitHub client user lookups with
o.userCache.GetUsercentralizes user retrieval and reduces redundant API calls.Also applies to: 404-404
427-433: LGTM!The updated
orgRoleBuildersignature correctly accepts and wiresuserCacheinto the resource type.pkg/connector/repository.go (4)
63-64: LGTM!Adding
userCacheandteamCachetorepositoryResourceTypeenables cache-backed retrieval of users and teams, reducing redundant API calls.
187-187: LGTM!The updated error wrapping consistently propagates rate-limit details from the response, aligning with the PR's goal of improved observability.
Also applies to: 243-243
324-324: LGTM!Replacing direct client calls with
userCache.GetUserandteamCache.GetTeamcentralizes retrieval logic and reduces API traffic.Also applies to: 341-341, 389-389, 399-399
420-427: LGTM!The updated
repositoryBuildersignature correctly accepts and wires bothuserCacheandteamCacheinto the resource type.pkg/connector/enterprise_role.go (3)
25-25: LGTM!Adding
userCachetoenterpriseRoleResourceTypefollows the consistent pattern across all resource types in this PR.
143-143: LGTM!Replacing direct GitHub client lookup with
o.userCache.GetUserByLoginand updating error wrapping to include rate-limit information are both consistent improvements.Also applies to: 146-146
166-175: LGTM!The updated
enterpriseRoleBuildersignature correctly accepts and wiresuserCacheinto the resource type.
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
We're going to wait for the new baton-sdk caching changes to land before merging this.
Summary by CodeRabbit
Improvements
Bug Fixes